Refactor hyperdrive create and update commands to match each other's behavior and address bug with individual parameters on creation #7024

xortive · 2024-10-18T15:04:07Z

What this PR solves / how to test

Fixes SQC-338

fix: make individual parameters work for `wrangler hyperdrive create` when not using HoA

`wrangler hyperdrive create` individual parameters were not setting the database name correctly when calling the api.

refactor: use same param parsing code for `wrangler hyperdrive create` and `wrangler hyperdrive update`

ensures that going forward, both commands support the same features and have the same names for config flags

feature: allow using a connection string when updating hyperdrive configs

both `hyperdrive create` and `hyperdrive update` now support updating configs with connection strings.

Author has addressed the following

Tests
- TODO (before merge)
- Tests included
- Tests not necessary because:
E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required
- Not required because: no e2e's for this path (only hyperdrive dev) and should be adequately covered by unit tests
Changeset (Changeset guidelines)
- TODO (before merge)
- Changeset included
- Changeset not necessary because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s): [Hyperdrive] Update wrangler command documentation for hyperdrive cloudflare-docs#17840
- Documentation not necessary because: no changes

changeset-bot · 2024-10-18T15:04:10Z

🦋 Changeset detected

Latest commit: 0db8edf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
wrangler	Minor
@cloudflare/vitest-pool-workers	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

ReppCodes · 2024-10-18T15:07:45Z

packages/wrangler/src/hyperdrive/client.ts


+export type OriginHoA = OriginCommon & {
+	access_client_id: string;
+	access_client_secret?: never;


Having the base OriginHoA include the secrets, when we have a WithSecrets version, seems wrong to me?

OriginHoA is actually enforcing that the secret is not present here, this type is saying "access_client_secret" must never be set.

ReppCodes · 2024-10-18T15:08:57Z

packages/wrangler/src/hyperdrive/client.ts

+	port?: never;
+};
+
+export type OriginHostAndPort = OriginCommon & {


In SQC-Config, the "HostAndPort" naming convention is what we use for non-HoA configs. Having the HoA-specific fields here seems wrong?

This is doing the same thing as OriginHoA, instead enforcing that the HoA fields are not present

github-actions · 2024-10-18T15:11:18Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11572515343/npm-package-wrangler-7024

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7024/npm-package-wrangler-7024

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11572515343/npm-package-wrangler-7024 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11572515343/npm-package-create-cloudflare-7024 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11572515343/npm-package-cloudflare-kv-asset-handler-7024

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11572515343/npm-package-miniflare-7024

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11572515343/npm-package-cloudflare-pages-shared-7024

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11572515343/npm-package-cloudflare-vitest-pool-workers-7024

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11572515343/npm-package-cloudflare-workers-editor-shared-7024

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11572515343/npm-package-cloudflare-workers-shared-7024

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11572515343/npm-package-cloudflare-workflows-shared-7024

Note that these links will no longer work once the GitHub Actions artifact expires.

[email protected] includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20241022.0
`workerd`	1.20241022.0	1.20241022.0
`workerd --version`	1.20241022.0	2024-10-22

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

ReppCodes · 2024-10-18T15:20:58Z

Overall I love the refactor, much cleaner and should be way easier to maintain going forward. I just think the ClientType names and contents need a once-over to align with the config service a bit more.

ReppCodes · 2024-10-24T18:36:36Z

Looks good to me from the Hyperdrive Team side of things.

emily-shen

Tests are also currently failing - think its just test snapshots need to be updated though (run them locally and press u)

packages/wrangler/src/__tests__/hyperdrive.test.ts

emily-shen · 2024-10-28T19:29:33Z

packages/wrangler/src/hyperdrive/index.ts

+			throw new UserError(
+				"You must provide a password - e.g. 'user:[email protected]:port/databasename' "
+			);
+		}


ideally the user doesn't have to incrementally fix things and get new error messages each time - can you run all these checks and list all errors once at the end?

We have some arguments like --port where you need to provide --port OR --access-client-id and --access-client-secret that makes doing that complex, and I don't think there will be an improvement in user experience that justifies the work required as a result

wont block on this, but for future reference, in the same place where you currently throw you can push the error messages to an array, then join them back up and throw once after the if/else checks :)

emily-shen · 2024-10-28T19:47:54Z

packages/wrangler/src/hyperdrive/index.ts

+		if (!args.originScheme || args.originScheme === "") {
+			throw new UserError(
+				"You must specify the database protocol as --origin-scheme - e.g. 'postgresql'"
+			);
+		} else if (!args.database || args.database === "") {
+			throw new UserError("You must provide a database name");
+		} else if (!args.originUser || args.originUser === "") {
+			throw new UserError(
+				"You must provide a username for the origin database"
+			);
+		} else if (!args.originPassword || args.originPassword === "") {
+			throw new UserError(
+				"You must provide a password for the origin database"
+			);
+		}


this should also check if all the required args are present and error once with all the missing args

packages/wrangler/src/hyperdrive/index.ts

…behavior and address bug with individual parameters on creation

xortive requested a review from a team as a code owner October 18, 2024 15:04

ReppCodes reviewed Oct 18, 2024

View reviewed changes

xortive force-pushed the malonso/wrangler-hyperdrive-create-update branch from d6eabca to da3efac Compare October 24, 2024 18:34

emily-shen requested changes Oct 28, 2024

View reviewed changes

Refactor hyperdrive create and update commands to match each other's …

3589173

…behavior and address bug with individual parameters on creation

xortive force-pushed the malonso/wrangler-hyperdrive-create-update branch from da3efac to 3589173 Compare October 28, 2024 21:04

fix snapshots

0db8edf

emily-shen approved these changes Oct 29, 2024

View reviewed changes

emily-shen merged commit bd66d51 into cloudflare:main Oct 29, 2024
17 of 18 checks passed

workers-devprod mentioned this pull request Oct 29, 2024

Version Packages #7089

Merged

Refactor hyperdrive create and update commands to match each other's behavior and address bug with individual parameters on creation #7024

Refactor hyperdrive create and update commands to match each other's behavior and address bug with individual parameters on creation #7024

Conversation

xortive commented Oct 18, 2024 • edited by emily-shen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR solves / how to test

Author has addressed the following

Uh oh!

changeset-bot bot commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

ReppCodes Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

xortive Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ReppCodes Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

xortive Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ReppCodes commented Oct 18, 2024

Uh oh!

ReppCodes commented Oct 24, 2024

Uh oh!

emily-shen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

emily-shen Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

xortive Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

emily-shen Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

emily-shen Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xortive commented Oct 18, 2024 •

edited by emily-shen

Loading

changeset-bot bot commented Oct 18, 2024 •

edited

Loading

xortive Oct 24, 2024 •

edited

Loading

xortive Oct 24, 2024 •

edited

Loading

github-actions bot commented Oct 18, 2024 •

edited

Loading